time skipping supports max skip, polling, describe - #835
Conversation
| google.protobuf.Timestamp fast_forward_target_time = 2; | ||
|
|
||
| // The initial skip count. It only propagates across a chain of runs within the same execution. | ||
| int32 initial_skip_count = 3; |
There was a problem hiding this comment.
for reviewers: this shall be propagated so that we can stop workflow retry as workflow retry generates a new run instead retrying within the same run
085a333 to
67f341b
Compare
| // | ||
| // If this field is not set, the server applies a large default value (e.g. 100). The default can | ||
| // be changed through dynamic config, and is overridden by this field when set. | ||
| int32 max_skip_per_session = 4; |
There was a problem hiding this comment.
we use a per_session max_skip instead of a total/delta max_skip to make this feature easy
the unneeded complexity introduced by other options:
- total: when the value is updated, both the client and server need to read current skip number to decide if the new value is valid and we may need to expose the current skip number
- delta: as we set a default value when the field is not set, if the user set TSC multiple times, delta may silently accumulate the total number to a large number
| // The fast-forward that was active when the poll started reached its target time and completed. | ||
| RESULT_FAST_FORWARD_COMPLETED = 2; | ||
| // No fast-forward was in progress when the poll started, so there was nothing to wait for. | ||
| RESULT_NO_PENDING_FAST_FORWARD = 3; |
There was a problem hiding this comment.
also need to add an result of workflow end (status = completed/failed/canceled/TERMINATED/TIMED_OUT)
but we need the whole execution (a chain of runs) to end instead of a single run's status
|
|
||
| message PollWorkflowExecutionTimeSkippingResponse { | ||
| enum Result { | ||
| // The poll timed out server-side before any time-skipping state change occurred to fast forward. |
There was a problem hiding this comment.
Sound like this should be different than RESULT_UNSPECIFIED, maybe RESULT_TIMED_OUT?
| // In-flight work includes activities, child workflows, Nexus operations, signal/cancel external workflow operations, etc. | ||
| // User timers are not classified as in-flight work and will be skipped over; the virtual clock may also skip to the | ||
| // time point of the registered fast-forward when there is no in-flight work. | ||
| // Every time time is skipped, the skip count is incremented by one; max_skip_per_session bounds the number of skips allowed within a single time-skipping session. |
2a371d8 to
dd493a2
Compare
| google.protobuf.Duration fast_forward = 2; | ||
|
|
||
| // A client-supplied ID that must be set together with `fast_forward`. It is used to poll for | ||
| // fast-forward completion via PollWorkflowExecutionTimeSkipping. |
There was a problem hiding this comment.
for reviewers: explicitly making this id required if fast_forward is used for simplicity
There was a problem hiding this comment.
Is there a reason not to make it optional? The SDK only needs this to wait on the result of a FF, and it doesn't always need to do that.
There was a problem hiding this comment.
I think both ways work:
-
server doesn't allow empty ff-id, and if users don't want to wait on it they can set a default id or random id
-
server allow setting empty ff-id, and we need to define a sound behavior for polling with ff-id empty. I think the simplest way is we don't allow polling an empty ff-id
- 2.1. so polling with ff-id in the request will have an argument error
- 2.2. polling with a valid ff-id but the current ff-id is empty will return ff-id not matching
- 2.3. in comments we indicate if users want to poll for the ff they need to set a ff-id for it
dd493a2 to
b4113e7
Compare
| // If the execution is actively trying to skip time automatically when there is a chance, | ||
| // this field will be set to true. If time has stopped skipping either by fast-forward completion, | ||
| // max skip allowed checking, or user configuration, it will be false. | ||
| bool is_running = 2; |
There was a problem hiding this comment.
d9062aa to
226b1a5
Compare
…t-forward verification
…ed distinctions between types, which reduces complexity.
226b1a5 to
588c15b
Compare
a89ee5e to
02f4001
Compare
a373f49 to
111f563
Compare
29f4ec6 to
4c38398
Compare
5bf50f9 to
134dd43
Compare
134dd43 to
d01e17f
Compare
| FastForwardConfig fast_forward_config = 2; | ||
|
|
||
| // By default, executions started by another execution (e.g. a child workflow of a parent workflow or | ||
| // a schedule with the timeskipping policy enabled), inherit the "enabled" flag and skip time when possible. |
There was a problem hiding this comment.
refine the comment: this is about config propagation for different executions (not different runs within the same execution
| // This flag disables that inheritance. | ||
| bool disable_propagation = 3; | ||
|
|
||
| // The maximum number of skips allowed every time this field is updated. It protects the execution from |
| google.protobuf.Duration fast_forward = 2; | ||
|
|
||
| // A client-supplied ID that must be set together with `fast_forward`. It is used to poll for | ||
| // fast-forward completion via PollWorkflowExecutionTimeSkipping. |
There was a problem hiding this comment.
Is there a reason not to make it optional? The SDK only needs this to wait on the result of a FF, and it doesn't always need to do that.
| // The fast-forward that was active when the poll started reached its target time and completed. | ||
| // The poll timed out server-side before the fast-forward completed. The caller may poll again. | ||
| RESULT_POLL_TIMEOUT = 1; | ||
| // The fast-forward identified by the request's `fast_forward_id` reached its target time and completed. |
There was a problem hiding this comment.
Maybe repeat here that the server only keeps the most recently started FF ID.
| // max_session_skip_count, time skipping stops. Whenever this config field is updated, the accumulated | ||
| // skip count is cleared, marking the start of a new session. | ||
| // For an execution with a chain of runs (retry, cron, continue-as-new), the count is accumulated | ||
| // across all runs within the same session. |
There was a problem hiding this comment.
I wondering about an edge case where you execute a single fast forward, but across e.g. 3 retries: does this count as 3 skips against the max, or just one?
If it's just one, perhaps add something like "multiple retries during a single fast-forward only count as one skip".
There was a problem hiding this comment.
it will count as 3, and the default value right now is 250. I think it will be fine users can change the default value or override the value using api. But indeed it is hard for user to estimate the number accurately if they don't want to study the internal server timer mechanism so it will be rough number.
There was a problem hiding this comment.
How about a workflow that does 3 multiple sequential sleeps? Does each one count against the max?
I realize that this field is probably not one that users will set very often, but I want to figure out a way to write a test for it, if the SDK is going to expose it.
There was a problem hiding this comment.
Or perhaps it is enough to test this by setting the field, and reading the configuration back and confirming it? Rather than verifying server behavior?
There was a problem hiding this comment.
-
skipping 3 sleeps will count as 3. server won't skip multiple timers at each skip.
-
I agree that users most of the time don't need to feel the existence of this option. And even if this max skip takes effect, they may not want to set this field but to figure out if their tests have unlimited retires.
What changed?
TimeSkippingConfigTimeSkippingInfotoDescribeWorkflowExecution(contains virtual current time and time-skipping runtime status)PollWorkflowExecutionTimeSkippingfor fast-forward completionfast_forwardfrom a duration field to a message with both duration and idBreaking changes
Change #5
Server PR
temporalio/temporal#11220